fix(engine): preserve static dedup across caption runs - #3438

Verdict: comment-only. Static-dedup rewrite is well-structured, tests cover the key invariants I would have written, and CI is fully green at head across CodeQL, Test, Preflight, Lint, Format, Typecheck, Build, Producer unit + integration, Preview parity, Render on windows-latest, and all Perf lanes. Windows tests and 6 of 9 regression shards are still in flight. Not stamping (fresh review not yet requested by anyone with stamp authority, and I want the regression shards to land first). Notes below.

Focus-axis pass

1. Timing precedence — resolveAuthoredTimingWindow matches the spec:

• start required and finite; non-finite (Infinity/NaN/empty) rejects the whole window.
• duration: public > 0 → preserved > 0 → null. If a positive duration is picked, end = start + duration and the fn returns immediately (duration-derived end wins).
• end: public > start → preserved > start → null. end == start and end < start both fall through, so a zero-duration or reversed window is rejected. Good.
• parseElementDurationAttr / parseElementEndAttr in timeline.ts bind the fallback chain correctly (public first, preserved second).
• The two authoredTiming.test.ts cases plus frameCapture-staticDedupBoundary.test.ts cover: duration-beats-end precedence, blank / NaN / Infinity / zero / negative public duration all falling through to preserved duration, end < start falling through to preserved end, and start-only windows returning duration: null, end: null. I checked the numbers by hand for the two boundary tests and they match (25 fps → edges {0,1,87,88,89}; 23.976 fps → {1,2,3,6,7,8}). No off-by-one.

2. Partial-budget safety — bounded verifier correctly discriminates outcomes:

• time_budget / count_budget: verifiedFrames retains only fully-completed runs (frames added only after the completedRuns++ line, which fires only after every planned comparison for that run matched). Screenshot count and deadline are checked BEFORE each capture in seekCapture, so a partial run can't leak.
• mismatch / infrastructure (thrown capture): both call verifiedFrames.clear() and clear stats before returning — atomic full-clear. Confirmed by the frameCapture-staticDedupVerifyDensity.test.ts "clears earlier verified runs on %s" it.each covering both.
• hardCap = max(400, samples*8, ceil(predictedFrames/24)*3 + runs.length); for the caption-heavy case (predicted 570, 121 runs, 24 samples) → max(400, 192, 193) = 400. Never exceeds the wall deadline path.

3. Planner profitability / density — planStaticVerification:

• net savings = frameCount - comparisons.length - 1 — correct: comparisons are the interior+endpoint captures, -1 is the anchor capture.
• Unprofitable filter: anchor >= 0 && netSavings > 0. Note the anchor-< 0 case (run starts at frame 0) is bucketed under skippedRuns with reason: "unprofitable", which is a slight mislabel — semantically it's "no valid anchor available." Cosmetic.
• 24-frame gap invariant: comparisons are at anchor + 24, anchor + 48, … plus the run's end. Max gap between adjacent verification points ≤ 24 frames. The "keeps endpoints, a 24-frame max gap, a global floor" test asserts this directly. 24 is a FRAME count, so wall-time coverage varies with fps (1s at 24fps, 0.4s at 60fps). Not a bug; a doc note might help future callers reason about it.
• Top-up loop only adds midpoints to already-profitable runs and re-checks frameCount > comparisons.length + 2 to keep netSavings > 0. Sort is deterministic (widest gap first, then run.a, then left). Good.
• Ordering: kept runs sorted by descending netSavings before verification, so budget exhaustion retains the highest-value runs. Good.
• Byte-identity arming is per-run (any one comparison mismatch clears every armed frame), not per-frame — appropriate given the anchor is what gets reused.

4. Failure taxonomy — clean:

• Outcomes distinguishable in perf-summary: verified | time_budget | count_budget | mismatch | infrastructure propagate through staticDedupVerificationOutcome and get aggregated in aggregateDedup as a sorted deduped list.
• No retry loop on infra failure — the try/catch is around the whole verification and exits once. No budget burn on permanent infra faults.
• armStaticDedup collapses mismatch + infrastructure to staticDedupSkipReason = "verification_failed" at the coarse-cardinality skipReason axis, but keeps them separated in the log line and in the fine-grained staticDedupVerificationOutcome field. Correct trade-off.
• Minor mislabel: when plan.runs is empty (every run filtered as unprofitable), verifyStaticFramesSafe returns outcome: "verified" with verifiedFrames.size === 0, and armStaticDedup then sets skipReason = "verification_budget" and logs "disabled (verified before frame undefined; no run fully verified)". The skipReason and the ${verdict.badFrame} in the log line are wrong for this path. Not user-facing — dedup just doesn't arm — but the telemetry undercounts "unprofitable-plan" and overcounts "verification_budget." Consider a separate unprofitable_plan skipReason, or at minimum guard the log's ${verdict.badFrame} for undefined.

5. Code / API quality — good:

• Types are tight (RawAuthoredTiming, AuthoredTimingWindow, StaticVerificationRun, StaticVerificationPlan, StaticVerificationResult, exported outcome union).
• Dependency injection (dependencies: StaticVerificationDependencies with now and capture) is what makes the density tests deterministic without a real browser. Well done.
• computeStaticVerificationPoints is fully removed (grep confirms zero remaining references). No stragglers.
• resolveAuthoredTimingWindow reused across four call sites (startResolver duration read, startResolver end read, timeline parseElementDurationAttr, timeline parseElementEndAttr, engine computeAuthoredClipBoundaryFrames) → single source of precedence truth. Good.

Cross-checks beyond Magi's summary

• Perf claim (25.4% / 45.66s / 61.20s / 490 reused / 939/939 byte parity) — anecdotal. No CI-asserted perf lane runs this scenario, and the Perf: parity / drift / fps / load / scrub lanes are green but don't lock these specific numbers. Treat these as hand-measurement one-shots, not a regression floor.
• PR body diverges from your briefing on baseline: body reports 61.20s baseline / 72.81s corrected-boundary control → 45.66s. The "25.4% vs baseline" number bakes in a boundary-correctness improvement AND a perf improvement; the perf-work-only saving is closer to 72.81 → 45.66 (~37%). Not blocking, just clarifying.
• #3431 reference: your briefing said "fixes caption-dedup regression from #3431" — #3431 is the original bug report (still open), not a prior regression-introducing PR. This PR closes it, and the two-part bug (mounted-layer boundary + caption-heavy verification timeout) maps cleanly onto the two commits (ac5d2d3 boundary fix, dfe3c42 perf bound).
• Density old-vs-new: the OLD computeStaticVerificationPoints sampled [a, a+stride, …, b] from the run start; NEW anchors at a-1 and samples at [a-1+24, a-1+48, …, b]. Interior spacing is similar (~24 frames), but frame a itself is now unchecked. Still within the 24-frame gap invariant (anchor@a-1 to first comparison ≤ 24 frames). Not a correctness gap.
• Cross-reviewer: no prior reviews, no inline or issue comments, reviewDecision: REVIEW_REQUIRED, mergeStateStatus: BLOCKED.

CI status at dfe3c42

All completed checks green: CodeQL (actions / js-ts / python), Build, Lint, Format, Typecheck, Preflight, Perf drift/fps/load/parity/scrub, Preview parity, Producer unit + integration, Test, Test: runtime contract, SDK unit+contract+smoke, Render on windows-latest, Studio load smoke + timeline viewport gate, WIP, player-perf, preview-regression, regression-shards 1/3/9. In flight: Tests on windows-latest and regression-shards 2/4/5/6/7/8.

Suggestions (non-blocking)

• Add an unprofitable_plan skipReason (or unify the empty-plan path under ineligible) so the "no run cleared profitability" case doesn't get counted as budget-exhausted in telemetry. Also guard the ${verdict.badFrame} interpolation in the empty-plan log line — it currently prints undefined.
• Consider labeling anchor-< 0 skips with a distinct reason (e.g. no_anchor) rather than unprofitable, so anyone reading the perf summary can tell whether the schedule structurally missed the front edge vs failed the savings test.
• The 24-frame STATIC_VERIFY_REFERENCE_STRIDE is fps-agnostic in gap semantics; a brief comment noting that (and the wall-time it implies at typical fps) would help future tuning.
• A vitest lock on the empty-plan.runs path would fill the last gap in the outcome taxonomy tests.

— Review by tai (pr-review)

Additive review at exact head 247487bb3b2de726db66e56e87db662f1d8d7019. @terencecho reviewed at dfe3c4243 and flagged that parts might be moot at a newer head, so this cross-checks that, covers the two commits added since, and then goes at ground their pass didn't: the timeline.ts / startResolver.ts refactor's behavior at its existing call sites, and whether the new tests actually guard.

A note on method, because it shaped this review: the branch was force-pushed four times while I was reading it (dfe3c4243 → 5f48d15c2 → 59bf6502e → 247487bb3), each time folding fixes into the same two commits. I re-anchored on every move and diffed rather than re-reading, so the findings below are pinned to the head named above. Confirmed byte-identical across all of those moves: timeline.ts, startResolver.ts. authoredTiming.ts gained exactly one line, and frameCapture.ts gained the sample-floor bound and the mismatch telemetry reset.

Worth saying plainly: amending fixes into the same SHA during an active review round makes any SHA-pinned verdict stale within minutes, which is why @terencecho's pass and mine both had to open with a head caveat. Separate follow-up commits would let a reviewer confirm just the delta.

The stale-head finding is closed. All-unprofitable plans return early via if (plan.runs.length === 0) return finish("unprofitable"), and armStaticDedup has a dedicated unprofitable branch with its own skip reason and message. I also checked the follow-on hazard: the verifiedFrames.size === 0 branch interpolates verdict.badFrame, and it can no longer be reached with that undefined — verified implies a non-empty set (plan.runs is non-empty by then and every completed run adds at least one frame), so only time_budget and count_budget reach it and both pass a frame.

The negative-start clamp is a real correctness fix, not a parity tweak

const start = Math.max(0, parsedStart) is the whole code delta at this head, and it is worth spelling out because its value is easy to under-read. I verified the premise it rests on rather than taking the commit message for it: startResolver.ts clamps at every branch that produces a start — Math.max(0, expression.value) at 153, then 154, 166, 170 and 180 — so the runtime genuinely renders a negative data-start from zero.

The reason that matters is computeAuthoredClipBoundaryFrames, which is the one caller that passes a raw data-start attribute straight in without going through the runtime resolver. Before the clamp, data-start="-1" data-duration="3" at 25fps predicted edges at round(-25) (discarded as negative) and round((-1+3)*25) = 50, so the boundary set protected {49,50,51} while the clip actually disappears at frame 75. That is the #3431 failure mode exactly — an unguarded disappearance inside a run dedup then trusts as static. The new test pins it at {0,1,74,75,76}, and without the clamp it computes {49,50,51}, so it fails for the right reason.

Worth noting how narrowly targeted the fix is: at the other three call sites the clamp is a no-op, because timeline.ts and startResolver.ts's duration branch both pass a hardcoded start: 0, and startResolver's end branch passes a start that resolveStartForElementInternal has already clamped. So this changes behavior only where the parity gap actually was.

Strengths

• init.ts:461-471 is what makes the per-tier fallback sound: the normalization writes data-hf-authored-duration / data-hf-authored-end and then removes both data-duration and data-end, so public and preserved attrs are mutually exclusive in practice. The #3431 regression test pins the reported failure exactly — Math.round(3.5 * 25) is 88, giving {87, 88, 89}.
• The new adjacency tests are real guards, not decoration. I reasoned each as a mutation probe rather than trusting the names: delete the session.lastFrameAbsoluteIndex === absFrameIndex - 1 condition in captureFrameCore and "does not reuse an unrelated cached frame from an interleaved worker" goes red, because the reuse branch would return SENTINEL instead of falling through to the throwing capture. Drop the session.lastFrameAbsoluteIndex = absFrameIndex advance inside that branch and "advances the cached absolute index across consecutive reuse hits" goes red on its second call.
• The adjacency guard is fail-safe by construction across the three input classes I enumerated: a worker slice starting mid-run (its first frame captures fresh, and it is byte-equal to the anchor by that run's own verification, so the chain it seeds is still correct), gaps left in the verified set by partial arming (the frame before each retained run is never in the set, so it is always captured fresh), and two retained runs in sequence (runs are maximal-contiguous, so they are never adjacent). In all three a broken chain degrades to a fresh capture rather than a wrong reuse.
• Fail-closed holds on every exit I traced. catch clears verifiedFrames and resets both counters before returning infrastructure; mismatch clears before returning; and verifiedFrames is only ever populated after stats.completedRuns++, at the end of a run whose anchor and every comparison matched. So an exception thrown mid-plan cannot leave a partially-verified run armed.

Findings

1. timeline.ts:359 — the refactor changes behavior at one of its three call sites, undisclosed and unpinned. important, not blocking.

parseElementDurationAttr now returns null where it previously returned a non-positive number, because the resolver only accepts a duration that is > 0. Line 359 is the one call site whose control flow turns on exactly that distinction:

let duration = parseElementDurationAttr(node);
if (duration == null && nodeCompositionId && ...) { duration = resolveTimelineDurationSeconds(...) }

A clip authored data-duration="0" previously resolved to 0, which is not == null, so every fallback was skipped and if (duration == null || duration <= 0) continue dropped it — the clip never appeared in the timeline. Now it resolves to null, the timeline / natural-media / inherited-duration fallbacks all run, and the clip appears with an inherited duration. The sharper case is data-duration="0" alongside data-hf-authored-duration="3.5": the old parseNum(a) ?? parseNum(b) returned 0 (?? does not coalesce zero), the new code returns 3.5.

The resolver's own half of this is deliberate and tested ("zero public duration" and "negative public duration" both assert the preserved fallback). What isn't covered is the caller-level consequence in the Studio timeline model. The direction looks intended and this isn't the render path, which is why it isn't a blocker — but it is a semantic change riding inside a presented-as-mechanical refactor, with nothing pinning either behavior.

The other two call sites are provably unchanged, and I checked rather than assumed: line 258 is gated on rootDurationFromAttr > 0 at 283-286, and 599-604 already collapse <= 0 into the null branch — I walked data-end="0" and an end < start case through both revisions and they land on durationFromTimeline identically.

2. Cross-tier precedence has no real producer holding it in place. nit.

Because init.ts:470-471 removes data-duration and data-end together, "public wins, preserved falls back" is only ever exercised within a tier — every test here is within-tier too. The second boundary test pins a cross-tier case (duration="0" + authoredDuration="0.2" + end="9", where the preserved duration beats the public end) that normalization cannot actually produce. Fine as a unit test, but it means the outer "any duration beats any end" ordering is unconstrained, and a future writer that sets a public data-end on an already-normalized element would silently get preserved-over-public. Either assert the XOR invariant or say in the docstring that cross-tier order is arbitrary.

3. The sample-floor term in hardCap is now dead. nit.

The new bound is STATIC_VERIFY_MAX_GLOBAL_SAMPLE_FLOOR = STATIC_VERIFY_MIN_SCREENSHOT_CAP / STATIC_VERIFY_SAMPLE_CAP_MULTIPLIER, i.e. 50, and effectiveSampleFloor is clamped to it. So plan.effectiveSampleFloor * STATIC_VERIFY_SAMPLE_CAP_MULTIPLIER can never exceed STATIC_VERIFY_MIN_SCREENSHOT_CAP by construction, which makes that argument to Math.max provably never the maximum. Harmless, but it reads as though the knob still scales the cap when it no longer can — either drop the term or note that it is retained only for documentation. (The bound itself is right, and it closes a real hole: before it, a large HF_STATIC_DEDUP_SAMPLES inflated the screenshot cap linearly and defeated the budget it was supposed to enforce.)

4. startResolver.ts end branch does strictly more work. nit.

The old code computed resolveStartForElementInternal(element, 0) only when an end attribute was present; the new code computes it before building the resolver input, so elements with neither data-end nor data-hf-authored-end pay for a start resolution they discard. durationCache keeps it to once per element, so this is small — worth a guard only if it shows up.

On the byte-identical claim

Flagging what the diff guarantees against what the number reports, since the two read alike in the body. The code guarantees sampled equality: for every retained run, the anchor plus comparison points at gaps of at most 24 frames matched byte-for-byte. I confirmed the bound holds — mandatoryRunComparisons steps anchor + 24 while < end then always adds end, so no gap exceeds 24. It does not guarantee the frames between two comparison points, so a change that appears and reverts inside a 24-frame window stays invisible.

That is not a regression: the old computeStaticVerificationPoints scaled off the same 24-frame reference stride, so density is comparable. The point is only that "939/939 raw frames byte-identical" is a measured outcome on one composition, not a property the verifier enforces, and I would state it as the former.

Related, and correctly disclosed in the body rather than a finding: the real behavioral delta is that budget exhaustion changed from disarming everything to arming the fully-completed runs, so compositions that previously rendered with dedup entirely off will now arm their highest-savings runs. That is the intended win, and it also means the sampling caveat above applies to more renders than before.

I agree the headline percentage mixes the correctness fix with the perf work, and won't restate @terencecho's reasoning on it.

Verdict: COMMENT
Reasoning: No blockers. The planner's profitability and gap invariants, the fail-closed paths, and the adjacency guard all hold at this head; the stale-head finding is fixed; and the negative-start clamp is a genuine correctness fix whose premise I verified in startResolver.ts. The one item I'd want before merge is a parity assertion on the timeline.ts:359 behavior change. Not approving because I have no stamp authority on this PR — see the thread.

— Rames Jusso

Re-review at head 247487bb3 (two force-pushes since prior tai COMMENT at dfe3c4243; ~9 files touched, mostly under packages/engine/src/services/frameCapture{,-*}.ts + one authoredTiming clamp in core). Both prior findings landed; adding this substantive re-review because both fixes carry regression tests I traced end-to-end.

Empty-plan fix — verified. verifyStaticFramesSafe now short-circuits plan.runs.length === 0 to outcome: "unprofitable" (packages/engine/src/services/frameCapture.ts:3035), the outcome union is widened (:2950-2956) and mirrored in CapturePerfSummary (packages/engine/src/types.ts:296-310). armStaticDedup handles the new outcome BEFORE the verifiedFrames.size === 0 branch (:3186-3190) — the misleading "verified before frame undefined; no run fully verified" + skipReason=verification_budget combo can no longer fire for an all-unprofitable comp. Regression test asserts outcome==="unprofitable" && verifiedFrames.size===0 && plannedRuns===0 at frameCapture-staticDedupVerifyDensity.test.ts:256-268.

Direct-predecessor reuse fix — verified. New CaptureSession.lastFrameAbsoluteIndex (:93) tracks the absolute index of the retained buffer. Serial-path reuse gate now requires session.lastFrameAbsoluteIndex === absFrameIndex - 1 (:3336-3340), and advances the tracker on BOTH reuse (:3342) and fresh capture (:3473). Correctly mirrored around the warmup capture (backup/restore at :3843/3855) and cleared in prepareCaptureSessionForReuse (:3944) so a same-composition reuse re-seeds on its first real capture rather than inheriting a stale index. Pre-fix bug: a worker whose lastFrameBuffer was pinned to frame N could reuse it for any later static frame M in the same run without proof that frames N+1..M-1 were also captured (or verified static) through this worker — interleaved-parallel schedules and warmup discards were the concrete leak paths. New gate collapses to hasIt(M) && lastIdx===M-1, i.e. the invariant the serial path can actually attest to; the pipelined worker path already carries the equivalent guard via its lastEncodeResultFrame + gap-static loop (:3378-3401), left unchanged.

Consecutive reuse-index advancement — verified. Advancement is unconditional on both branches — reuse advances (:3342), fresh capture advances (:3473), warmup discard rolls back (:3855). Regression: frameCapture-staticDedupIndex.test.ts:86-93 seeds staticFrames={90,91}, lastFrameAbsoluteIndex=89 and asserts two consecutive reuses land with lastFrameAbsoluteIndex===91 && staticDedupCount===2 — kills the off-by-one variant where advancement was only on capture. Companion test at :78-83 pins the non-predecessor reject: staticFrames={90}, lastFrameAbsoluteIndex=87 correctly falls through to the page-trap throw.

Follow-on hardening at 247487bb3. Small additive delta on top of the fixes:

• planStaticVerification now normalizes + clamps sampleFloor — non-finite→1, else max(1, floor(input)), then capped by STATIC_VERIFY_MAX_GLOBAL_SAMPLE_FLOOR = STATIC_VERIFY_MIN_SCREENSHOT_CAP / STATIC_VERIFY_SAMPLE_CAP_MULTIPLIER = 50 (:2842-2845, :2916-2921). Exposes effectiveSampleFloor on the plan (:2862, :2953). verifyStaticFramesSafe uses this clamped value for hardCap (:3062). Bounds pathological callers (large HF_STATIC_DEDUP_SAMPLES) without touching the default-24 or ≤50 path — for effectiveSampleFloor ≤ 50, hardCap = max(400, floor·8, per-stride) reduces to the same 400-base cap the code has always used.
• On mismatch, verifier now also zeroes stats.verifiedFrames/stats.unverifiedFrames = predictedFrames (:3088-3089), aligning stats.* with the cleared verifiedFrames Set (previously the set was cleared but the running per-run stats were not). Pinned by test at frameCapture-staticDedupVerifyDensity.test.ts:289-291.

4-axis pass at new head — still clean.

• Timing precedence. resolveAuthoredTimingWindow now Math.max(0, parsedStart)s a finite negative start (packages/core/src/runtime/authoredTiming.ts:24-28) so computeAuthoredClipBoundaryFrames matches the runtime clamping — new test at frameCapture-staticDedupBoundary.test.ts:39-43 pins the {start:"-1",duration:"3"}→{0,1,74,75,76} boundary set at fps=25. Additive-only clamp; doesn't touch the precedence tree.
• Partial-budget safety. time_budget/count_budget still return the accumulated verifiedFrames (verifier retains completed runs; only mismatch/infrastructure clear the set). armStaticDedup still arms on any non-empty verifiedFrames.size (:3198), so a partial-budget verdict remains armable.
• Planner. planStaticVerification retains the netSavings > 0 && anchor >= 0 filter (:2906), unprofitable diverts to skippedRuns with typed reason (:2907-2909), 24-frame STATIC_VERIFY_REFERENCE_STRIDE and DESC-netSavings ordering (:2831, :2937) unchanged.
• Failure taxonomy. Six outcomes now (verified, unprofitable, time_budget, count_budget, mismatch, infrastructure) each with a distinct skipReason mapping in armStaticDedup. No lossy collapse.

Fresh full-diff pass. No new issue. authoredTiming clamp is a strict tightening; new capture-session field is monotone; new outcome/skipReason strings are correctly plumbed through types.ts and the perfSummary aggregator; sample-floor clamp is a no-op at defaults and a strict tightening at extremes.

CI in flight at head, no red conclusions on eligible checks. Miguel explicitly authorized stamp at current head + merge on required-green; approving on that authorization.

— Review by tai (pr-review)